Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: Pre-calculated constant for convertLandauFwhmToGaussianSigma #2372

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 15, 2023

Pulled out of #2366 to pin down the hash and physmon diff

blocked by

@andiwand andiwand added this to the next milestone Aug 15, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Infrastructure Changes to build tools, continous integration, ... Component - Examples Affects the Examples module Changes Performance labels Aug 15, 2023
@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #2372 (17bb728) into main (a2ddfc9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2372   +/-   ##
=======================================
  Coverage   49.58%   49.58%           
=======================================
  Files         453      453           
  Lines       25517    25517           
  Branches    11706    11706           
=======================================
  Hits        12653    12653           
  Misses       4581     4581           
  Partials     8283     8283           
Files Changed Coverage Δ
Core/src/Material/Interactions.cpp 74.19% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions github-actions bot removed Infrastructure Changes to build tools, continous integration, ... Component - Examples Affects the Examples module Changes Performance labels Aug 15, 2023
@felix-russo
Copy link
Contributor

Any idea why we reconstruct less vertices now?
image

@andiwand
Copy link
Contributor Author

not sure if we do - the plot is hard to read

these changes should not affect the AMVF results but do for unknown reasons. this is what I experienced a couple of times already. small changes in the sim or track fitting result in big changes in the AMVF hist cmp

I updated the reference because the problem seems unrelated

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Component - Examples Affects the Examples module Changes Performance labels Aug 16, 2023
@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Aug 16, 2023
@github-actions github-actions bot removed the Infrastructure Changes to build tools, continous integration, ... label Aug 16, 2023
kodiakhq bot pushed a commit that referenced this pull request Aug 17, 2023
AMVF vertex finding dropped vertex candidates because `isMergedVertex` would return `true` for negative covariance matrices.

The reason why these matrices are negative definite is still unclear but we should not consider them as merged vertices by default I think.

This was the reason why we experienced noisy AMVF physmon diff for mostly unrelated changes because these conditions would flip flop on small numerical changes. A small reproducible example can be found in #2372
benjaminhuth
benjaminhuth previously approved these changes Aug 17, 2023
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its a shame that things like std::log only get constexpr with C++26...
But looks good, let's get it in!

@benjaminhuth benjaminhuth added automerge and removed 🛑 blocked This item is blocked by another item labels Aug 17, 2023
@paulgessinger
Copy link
Member

physmon and the examples tests still seem to fail.

@andiwand
Copy link
Contributor Author

oh no... the hash changes are expected but I would have thought the AMVF diff should be gone by now 😞

@benjaminhuth
Copy link
Member

Really weird...

An alternative would be a const static float variable in the function... this would then only be computed once at runtime, but should be identical?

@andiwand
Copy link
Contributor Author

I am trying to find the problem in the AMVF which is causing this flip flop of vertices for small numerical changes. This diff here is quite nice to be able to find the spot

@kodiakhq kodiakhq bot merged commit ac741c2 into acts-project:main Aug 21, 2023
54 checks passed
@andiwand andiwand deleted the minor-interactions-precalc-constant branch August 21, 2023 08:02
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Aug 21, 2023
kodiakhq bot pushed a commit that referenced this pull request Aug 21, 2023
…pes (#2366)

Refactors the interactions interface to be minimal. That means only passing absolute charge and absolute PDG and only where necessary.

At the same time I made the types explicit. For some reason we use `float` all across the interaction code which I kept and made more explicit.

also fixes a weird FPE discovered in
- #2336

pulled these changes out of
- #2341
- #2336

blocked by
- #2375
- #2372
@paulgessinger paulgessinger modified the milestones: next, v29.0.0 Aug 22, 2023
@paulgessinger
Copy link
Member

Athena changes did not come from this PR.

@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants